Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Update AccessToken #36406

Closed
wants to merge 1 commit into from
Closed

Conversation

pvaneck
Copy link
Member

@pvaneck pvaneck commented Jul 10, 2024

Earlier, refresh_on was added to AccessToken as an optional property. This had unintended breaking side effects. The azure-servicebus package recently ran into issues when they were unpacking the get_token() call:

access_token, self._token_expiry_on = self._credential.get_token(self._endpoint)

This yielded: ValueError: too many values to unpack (expected 2).

To avoid breaking users who might be unpacking like this, this change converts AccessToken into a DataClass. A NamedTuple and frozen DataClass are functionally the same for our purposes, but a DataClass provides more flexibility. Here, I override the __iter__, __getitem__, and __len__ methods to provide the same functionality as previously.


Why we need refresh_on?

In some cases, our current strategy of requesting a new token if it is within five minutes of expiration is not sufficient. Some customers have long-running jobs that span longer than five minutes and would like earlier refresh times to avoid authentication issues causing interruptions. In some cases, tokens can contain info on when it can/should be refreshed. We want to be able to use that info in our BearerTokenCredentialPolicies.

Issue: #35473

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-core

@pvaneck pvaneck force-pushed the core-accesstoken-unpack branch 6 times, most recently from 0a9912e to f7b332f Compare July 10, 2024 22:24
@pvaneck
Copy link
Member Author

pvaneck commented Jul 11, 2024

This change should avoid breaking people using AccessToken unless others can think of more scenarios that I can test.

The following usage scenarios are tested and still work as it did previously:

access_token = AccessToken("token", 42)
assert access_token.token == "token"
assert access_token.expires_on == 42

Unpacking (not a common occurrence, at least in our SDKs) still works as before:

token, expires_on = AccessToken("token, 42")
assert token == "token"
assert expires_on == 42

# Unpacking all values still works, but this assumes refresh_on is set:
token, expires_on, refresh_on = AccessToken("token", 42, 21)
assert token == "token"
assert expires_on == 42
assert refresh_on == 21

For the rare occurrence of people using numeric-indices to access values, this still works

token = AccessToken("token", 42)
assert token[0] == "token"
assert token[1] == 42
assert token[:2] == ("token", 42)

The BearerTokenCredentialPolicies will still work even if refresh_on doesn't exist as a property for the saved token in the case people implement their own credentials that return a different (non-core) declaration of an AccessToken (even though typing might not be happy, it'll still work). There is also a test for this.

Azure CLI does not import Azure Core's AccessToken or use Azure Core's BearerTokenCredentialPolicy, so no qualms there.

@pvaneck pvaneck force-pushed the core-accesstoken-unpack branch 3 times, most recently from 205f289 to a1881e0 Compare July 11, 2024 18:35
This allows adding a new field to AccessToken without breaking
unpacking or index-based access of the previous credential.

Signed-off-by: Paul Van Eck <[email protected]>
@pvaneck pvaneck marked this pull request as ready for review July 11, 2024 20:09
@pvaneck pvaneck changed the title [Core] Convert AccessToken to dataclass [Core] Update AccessToken Jul 11, 2024
@pvaneck pvaneck changed the title [Core] Update AccessToken [Core] Update AccessToken Jul 11, 2024
@johanste
Copy link
Member

@catalinaperalta , we should make sure that our breaking change tool catches additions of values to named tuples are flagged as breaking....

@pvaneck
Copy link
Member Author

pvaneck commented Jul 11, 2024

The following also works for all test cases if we would prefer to keep it NamedTuple:

class AccessToken(NamedTuple):
    """Represents an OAuth access token."""

    token: str
    expires_on: int
    refresh_on: Optional[int] = None

    def __iter__(self) -> Any:
        return (getattr(self, field) for field in self._fields if getattr(self, field) is not None)

    def __getitem__(self, index: int) -> Any:
        return tuple(self)[index]

    def __len__(self) -> int:
        return len(tuple(self.__iter__()))

The only caveat that I can see with this compared to dataclasses deals with typing:
image

Functionally, this still works.

@xiangyan99
Copy link
Member

@catalinaperalta , we should make sure that our breaking change tool catches additions of values to named tuples are flagged as breaking....

We will implement it in a non-breaking way which is not supposed to be caught.

assert token == "token"
assert expires_on == 42
assert refresh_on == 21

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also want to test the case:

token, expires_on = AccessToken(token="token", expires_on=42, refresh_on=21)
assert token == "token"
assert expires_on == 42

@pvaneck pvaneck closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants